Credential hand-over to per-user server sessions#1413
Credential hand-over to per-user server sessions#1413Piclaw (piclaw-bot) wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the IronRDP server/acceptor stack to support multi-user “sesman”-style architectures by handing authenticated credentials from the protocol handshake into a post-auth, per-user connection binding phase, while also handling deactivation/reactivation credential reuse safely on a per-TCP-connection basis.
Changes:
- Add a post-auth async
ConnectionBinderhook toironrdp-serverto swap in per-user display/input handlers after credentials are available. - Propagate CredSSP (NLA/Hybrid) delegated credentials into
AcceptorResult::credentialsso servers can validate/bind using the sameCredentialsshape as TLS ClientInfo. - Introduce a per-connection authenticated-credentials cache to support deactivation/reactivation flows where clients may omit a second credentials PDU.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/wrdp/reactivation-credential-cache.md | Documents the per-TCP-connection scope of the reactivation credential cache. |
| docs/wrdp/credssp-delegated-credentials.md | Documents CredSSP delegated credential handoff into AcceptorResult::credentials. |
| docs/wrdp/auth-delegation.md | Documents the post-auth connection binding model for multi-user servers. |
| crates/ironrdp-server/src/server.rs | Adds BoundConnection/ConnectionBinder, integrates binder + reactivation credential caching, and adds targeted tests. |
| crates/ironrdp-server/src/lib.rs | Re-exports the new binder/bound-connection API. |
| crates/ironrdp-server/src/builder.rs | Adds builder plumbing to configure the optional connection binder. |
| crates/ironrdp-acceptor/src/lib.rs | Captures CredSSP delegated credentials from the CredSSP sequence and stores them in the acceptor. |
| crates/ironrdp-acceptor/src/credssp.rs | Extends CredSSP result handling to return delegated credentials when the exchange finishes. |
| crates/ironrdp-acceptor/src/connection.rs | Updates AcceptorResult credential semantics and adds internal storage for CredSSP delegated credentials. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Benoît Cortier (CBenoit)
left a comment
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
Thank you!
This is covering a real dead angle in our API.
We generally prefer smaller, scoped PRs for each change, but since everything is intertwined I see where you come from.
The two added tests exercise resolve_authenticated_credentials in isolation with same-connection reuse and cross-connection isolation. Those are good and directly pin the #1412 scoping fix. However, the actual new surface is untested: the binder handler-replacement, the !reactivation skip (the #1411 fix), and the "binder set + no creds -> deny" branch. The reactivation-skip in particular is the behavior most likely to regress under future refactor, and it has zero coverage. A test with a fake ConnectionBinder asserting it fires once on initial accept and not on a synthesized reactivation AcceptorResult would lock the contract. This can be a follow up PR if you prefer.
| } | ||
|
|
||
| /// Set a binder that replaces display/input handlers after credentials are accepted. | ||
| pub fn with_connection_binder(mut self, binder: Option<Arc<dyn ConnectionBinder>>) -> Self { |
There was a problem hiding this comment.
thought: The Option param reads oddly at the call site (.with_connection_binder(Some(Arc::new(x)))), but it's consistent with with_credential_validator and with_connection_handler. This is consistent, but I’m wondering if we should change that. Out of scope for this PR.
There was a problem hiding this comment.
Agreed; I left this unchanged for this PR to stay consistent with the adjacent optional hooks and avoid mixing API ergonomics into the correctness fixes.
|
Hey there Benoît Cortier (@CBenoit)! Apologies for the longish PR, yes--I did try four smaller ones earlier, but then it was just simpler to get it all out in one chunk. I made sure my agent explained that when it got them together because, well, Copilot actually pointed out part of the inter-dependencies :) We'll add the follow-ups and tests for these in small, focused commits (sorry, had written PRs) - right now this is the minimal surface I need to get the "sesman" thing to work, and I think it really belongs here.
And regarding zeroing the credentials... Yeah. I know. I hate cacheing them in the first place, but I need to get them over to PAM in the actual RDP server for the user (which is in another context), and Windows App on the Mac tends to reconnect quickly for some reason, and... well, you know how this sort of thing goes. |
|
OK, after a bit of to and fro, and while Piclaw (@piclaw-bot) tidies up the mess I made, I would like to know if you're interested in us looking at other ways of handling credentials - I am only looking at passwords for Linux, but I can do Yubikeys as well - later, though. |
|
I'm glad to see this land as a single coherent PR. wrdp is the second standalone validate-on-receipt server I've watched drive real requirements into ironrdp-server, and it sits on a design axis I've spent a while building out. I want to connect it to the prior work and then flag one decision worth settling before it sets. Some context on where this sits. Last month I wrote #1357 ("Server-side credential handling: a design frame for the validate-on-receipt deployment class") to name this class. The class is the server that authenticates the connecting party itself rather than injecting pre-known vault credentials the way Devolutions Gateway does. That party might be an end user checked against PAM, LDAP, or AD, or a backend checked against a capability token. I run one of these in production, a single-process server with inline PAM, so the requirements you're hitting are familiar. The pieces of that class already in tree are the CredentialValidator (#1172, from issues #1154 and #1150), the AcceptorResult.credentials field this PR extends (added in #1155 for TLS-mode post-handshake validation), the lower-layer-TLS transport path (#1281, after the earlier #1210 attempt taught me not to misrepresent negotiated security on the wire), and the conformant-server-behind-a-proxy work (#1346, #1347, and #1348 in flight). I wrote #1357 in the same spirit as Benoît Cortier (@CBenoit)'s architecture-direction issues (#1349, #1352), and #508 is the same question arriving from outside. So there's a fair amount of prior design to hang this on, and your ConnectionBinder, which consolidates #1409, #1410, #1411, and #1412, is a natural and welcome third seam alongside transport and validation. The one thing I want to raise firmly, as the author of the CredentialValidator, is what happens to that validator once CredSSP delegated credentials flow through AcceptorResult.credentials. Before this PR the validator ran only on the TLS and Standard path, where credentials arrive unvalidated in the Client Info PDU and the validator is the authentication gate. That boundary was deliberate, and #1357 spells out why. CredSSP and NLA are a pre-session challenge-response that needs the password-equivalent secret in hand to complete, so they can't themselves validate against an opaque backend, and pam plus hybrid is incompatible by construction. The contract the validator makes with implementors is that these are unvalidated client-supplied credentials and the decision is accept or reject. This PR routes CredSSP delegated credentials into that same AcceptorResult.credentials field and through the same resolve_authenticated_credentials path, so a validator configured on a Hybrid server now fires on credentials CredSSP has already authenticated. Those two inputs have different provenance and different trust semantics, but the trait sees one field and can't tell them apart. A PAM-style validator written for the TLS path, following the documented contract, would silently begin gating Hybrid connections once this lands, which turns the incompatible-by-construction combination into one that quietly half-works. Copilot caught the now-stale doc line on with_credential_validator. My concern is the behavior change underneath it, and I wouldn't want to ship that silently. None of this blocks the capability, which is genuinely useful and, as you noted, covers a real dead angle. Exposing the delegated TSPasswordCreds so an embedding server can authorize them downstream is a correct pattern, and it's one #1357 didn't name because I was focused on the single-process TLS path. That's your sesman-style hand-off to PAM in the per-user context, and it should be folded in. What I'd ask is that the CredSSP-delegation path be explicit rather than implicit. Either the validator learns the provenance of what it's handed, TLS Client Info versus CredSSP-delegated, or the delegated-to-validator wiring becomes opt-in per security mode, so a validator authored for the TLS path doesn't silently start gating CredSSP-authenticated sessions. Your own binder guard already encodes the right instinct, since it requires either a validator or Hybrid, meaning the credentials were authenticated somehow. I'd just want that same distinction visible on the validator seam and not only in the binder. Two smaller notes. Borrowing the credentials from the current AcceptorResult rather than caching a clone for the connection lifetime, which I see you moved to after CBenoit's note, is the right default for this class, since secret lifetime is a live concern on the PAM path. And since the ConnectionBinder is a general ironrdp-server capability rather than a wrdp-specific one, the material under docs/wrdp/ might read better as generic API docs or rustdoc on the trait, so the next standalone-server author finds it where they'd look. The validator-provenance question is the one piece I'd want settled before this merges. The rest is additive, and it's good to see a second implementation pushing on this deployment class. Greg Lamberson |
There was a problem hiding this comment.
Thanks, the rework since the initial push addressed the substantive problems well. Dropping the credential cache in favor of borrowing from the current AcceptorResult is the right call for secret lifetime.
I see one item that remains before we merge this, then a few non-blocking notes.
issue: AcceptorResult::credentials needs a provenance tag.
This PR routes two different things into one field: unvalidated ClientInfo credentials (TLS/Standard path) and CredSSP-delegated credentials that the handshake already authenticated. Downstream, this shows up twice. First, a CredentialValidator configured on a Hybrid server now silently starts firing on credentials it was never written for: the trait's documented contract says "unvalidated client-supplied credentials", which is no longer always true (see Greg Lamberson (@glamberson)'s comment; I agree with the diagnosis). Second, the binder gate has to reconstruct "were these authenticated?" from RdpServerSecurity config instead of reading it from the data. That is correct today, but it silently depends on the negotiation invariants (Hybrid never selects SSL, ClientInfo creds suppressed under Hybrid) staying true.
suggestion: Please add an origin to the credentials, e.g.:
pub enum CredentialOrigin {
/// Received in the ClientInfoPdu (MS-RDPBCGR 2.2.1.11); not authenticated by the handshake.
ClientInfo,
/// Delegated TSPasswordCreds decrypted by CredSSP (MS-CSSP); authenticated by the exchange.
CredSspDelegated,
}either as a field alongside credentials or by wrapping the type. Then the binder gate checks the fact rather than the config, and the validator contract documents both cases explicitly. I do not want the validator suppressed on the delegated path as running authorization policy on an already-authenticated principal is, I think, coherent and is essentially the sesman pattern this PR exists for, but I want the implementor to be able to tell which case they're in.
suggestion: Clear the bound slots at connection entry, not only in run(). clear_bound_connection is currently called only from the run() accept loop. An embedder driving run_connection/run_connection_with directly who clears the binder mid-life (set_connection_binder(None)) would have a later connection reach the client loop with the previous user's handlers still installed in the slot. Clearing at connection entry makes the invariant local to every entry point.
suggestion: Document the serialization assumption in the slot wrappers. BoundDisplaySlot/BoundInputSlot take the box out of the std::sync::Mutex, .await, then put it back. This is only safe because the outer tokio::Mutex on self.display/self.handler serializes all callers. Please add a comment stating that; the .expect("lock poisoned") calls would otherwise turn a future violation into a panic in the input path with no explanation for whoever hits it.
suggestion: move docs/wrdp/*.md into rustdoc. ConnectionBinder is a general ironrdp-server capability, not a wrdp-specific one. The next standalone-server author will look for this on the trait, not in a docs/wrdp/ folder.
With the provenance tag in place, I'm happy to approve! 🙂
OK, after a bit of to and fro, and while Piclaw (Piclaw (@piclaw-bot)) tidies up the mess I made, I would like to know if you're interested in us looking at other ways of handling credentials - I am only looking at passwords for Linux, but I can do Yubikeys as well - later, though.
Yes, we would be very glad to merge more of this. When you have the time! Thanks!
EDIT: Also CI is red. You can check locally that everything is okay by running cargo xtask ci.
|
Hey there again! I very much like the notion of adding an origin to the credentials - especially because that may well help with messy PAM "routing" on the end client context. :) I'll think about whether there is anything along those lines that ties into the session management stuff I'm refactoring, then dispatch the agent to do both a) a targeted fix to what you point out and b) assess CI If there is anything else, I'll do another PR after this one goes through - I don't want to make this more complicated than it is, just better :) |
|
Thank you very much! Rui Carmo (@rcarmo) Also, would you prefer if we merge this PR as-is first (once the CI is green), and then send follow up PRs for the rest? |
|
Let's do the fixes first, and close this one... - I have no clear isolation for the rest of the stuff yet (I'm still poking at how to do parts of the session/protocol management and understanding if they have a place upstream here), and I'd rather submit reusable stuff than hacks :) |
Summary
This replaces and consolidates #1409, #1410, #1411, and #1412 into a single branch from
rcarmo:wrdp.The split PR heads use
wrdp/*branch names, but the fork now uses the canonicalwrdpbranch. Because Git cannot keep bothrefs/heads/wrdpandrefs/heads/wrdp/*at the same time, this PR carries the updated commits and review fixes together.Why
wrdpfollows thexrdp-sesmanmulti-user architecture model, so it needs a way to hand authenticated credentials from the protocol handshake into the per-user session binder.Changes
ironrdp-server.Credentialsshape used by ClientInfo.Copilot review follow-up
This includes fixes for the Copilot comments from the superseded PRs:
Validation
cargo check -p ironrdp-server -p ironrdp-acceptorgit diff --check